Skip to content

Consolidate buffer store translation#7251

Merged
pow2clk merged 4 commits intomicrosoft:mainfrom
pow2clk:longvec_bab_ldst_pr
Mar 24, 2025
Merged

Consolidate buffer store translation#7251
pow2clk merged 4 commits intomicrosoft:mainfrom
pow2clk:longvec_bab_ldst_pr

Conversation

@pow2clk
Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk commented Mar 24, 2025

Consolidate buffer store translation

Added structured and types buffer support to TranslateStore and used it for all
such lowerings.

Includes IR and fcgl tests for the same in addition to recently added load/store tests that exercise this same code.

Greg Roth added 2 commits March 21, 2025 00:05
Added structured buffer support to TranslateStore and used it for all
such lowerings.
For raw and typed tests both fcgl and dxilgen
@pow2clk pow2clk requested a review from a team as a code owner March 24, 2025 11:19
@pow2clk pow2clk changed the title Longvec bab ldst pr Consolidate buffer store translation Mar 24, 2025
Comment thread lib/HLSL/HLOperationLower.cpp Outdated
}
DXASSERT(MatTy.getLoweredVectorType(false /*MemRepr*/) == val->getType(),
"helper type should match vectorized matrix");
#endif
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#endif

Comment thread lib/HLSL/HLOperationLower.cpp Outdated
Comment on lines 7902 to 7903
#ifndef NDEBUG
HLMatrixType MatTy = HLMatrixType::cast(matType);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifndef NDEBUG
HLMatrixType MatTy = HLMatrixType::cast(matType);
[[maybe_unused]] HLMatrixType MatTy = HLMatrixType::cast(matType);

Response to feedback on debug code
@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 24, 2025

As a note for this and the preceding changes to load lowering, I've tested each against the behavior before any of this refactoring took place with the same tests and they pass as well. In a lot of cases, regression tests not failing without the change is bad, but in this case these are intended to be almost 100% NFC, but too involved to feel safe applying that flair.

Comment on lines +751 to +753
!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = !{!"hlsl-hlemit", !"hlsl-hlensure"}
!2 = !{!"dxc(private) 1.8.0.4820 (longvec_bab_ldst_pr, 243b35785)"}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't referenced and can be removed.

Note - The IR parser doesn't care if metadata nodes are contiguous, so you can just remove these and leave the numbers starting at 3 and that's fine. If you do want to renumber everything so that it's prettier, I recommend running the file through opt -S and copy pasting the updated metadata (from !dx.version on) back to this file.

Comment on lines +826 to +828
!98 = !{!99, !99, i64 0}
!99 = !{!"omnipotent char", !100, i64 0}
!100 = !{!"Simple C/C++ TBAA"}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These TBAA annotations can also be removed.

Comment on lines +1036 to +1038
!0 = !{i32 2, !"Debug Info Version", i32 3}
!1 = !{!"hlsl-hlemit", !"hlsl-hlensure"}
!2 = !{!"dxc(private) 1.8.0.4845 (disable_disble_spirv, 2514104b9-dirty)"}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These too

Comment on lines +1083 to +1087
!51 = !{!52, !52, i64 0}
!52 = !{!"omnipotent char", !53, i64 0}
!53 = !{!"Simple C/C++ TBAA"}
!69 = !{!70, !70, i64 0}
!70 = !{!"double", !52, i64 0}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these

Copy link
Copy Markdown
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore my one nit comment if it is a pain.

Comment thread lib/HLSL/HLOperationLower.cpp Outdated
DXASSERT_NOMSG(RK != DxilResource::Kind::StructuredBuffer);

OP::OpCode opcode = OP::OpCode::NumOpCodes;
bool isTyped = true;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since this isn't really new, but looks new due to refactoring, feel free to ignore if this is difficult to apply.

Suggested change
bool isTyped = true;
bool Typed = true;

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

@pow2clk pow2clk merged commit 9a06f4d into microsoft:main Mar 24, 2025
12 checks passed
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Mar 24, 2025
@pow2clk pow2clk deleted the longvec_bab_ldst_pr branch March 24, 2025 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants